-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: range without end #63
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰 완료합니다
test/timepicker/timepicker.spec.js
Outdated
|
||
timepickerNoMeridiem.setRange(start); | ||
|
||
selector = timepickerNoMeridiem.element.querySelectorAll('select[aria-label="Time"]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
실제 변수에 할당되는 결과물을 생각하면 selectBoxes
정도가 변수 이름으로 적절해보이네요.
test/timepicker/timepicker.spec.js
Outdated
selectOption = hourSelect.querySelector('option[value="8"]'); | ||
expect(selectOption.disabled).toBe(true); | ||
selectOption = hourSelect.querySelector('option[value="9"]'); | ||
expect(selectOption.disabled).toBe(false); | ||
|
||
timepickerNoMeridiem.setTime(9, 0); | ||
|
||
selectOption = minSelect.querySelector('option[value="30"]'); | ||
expect(selectOption.disabled).toBe(true); | ||
selectOption = minSelect.querySelector('option[value="31"]'); | ||
expect(selectOption.disabled).toBe(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트로 커버하고자 하는 범위가 더 명확했으면 좋겠습니다.
우리가 알고자 하는 것은 '(시간 선택의 경우) 8까지는 모두 disabled 이고, 9부터는 모두 enabled 이다' 입니다.
그런데 단순히 분기점 하나만 두고 이게 정상적으로 동작하는지 보장하는 것은 어폐가 있어 보이네요.
querySelectorAll
을 통해 모든 옵션을 가져온 뒤, 1 ~ 8까지 자르고, 9 ~ 12까지 자른 다음 각각의 배열이 원하는 disabled
값을 가지고 있는지 toMatchObject
단언을 활용해보면 좋겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hourSelect가 minSelect 둘다 같이 검증되어야 하는건가요? 만약 그게 아니라면 나누어서 검증하는게 좋을 것 같습니다. 그리고 시간이나 분을 범위 별로 검증(disabled
값)해야 한다면, 위의 리뷰처럼 범위 전체를 검증해야 정확하겠네요~! 테스트에서 클래스나 속성에 의해 자주 변경되는 요소가 아니라면 jest 스냅샷 테스트를 적용하는 것도 좋을 것 같아요.(만약 그렇지 않다면, toMatchObject
를 쓰는게 좋겠구요)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰완료합니다.
test/timepicker/timepicker.spec.js
Outdated
hourSelect = selector[0]; | ||
minSelect = selector[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[hourSelect, minSelect] = selector
로 줄이면 좋을 것 같습니다.
src/js/timepicker/index.js
Outdated
} else { | ||
this.disabledMinutes[beginHour] = util.getDisabledMinuteArr([disabledMinRanges[0]]).concat(); | ||
if (beginHour === endHour) { | ||
this.disabledMinutes[beginHour] = util.getDisabledMinuteArr(disabledMinRanges).concat(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concat()
을 사용한 이유가.. 배열을 복사하기 위한 용도로 사용된 것인가요~?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
복사 용도가 맞다면 concat()
보다는 slice(0)
을 사용하는 것이 좋을 것 같습니다. concat()
은 배열을 연결하는 목적의 함수이기 현재 사용 의도와는 맞지 않아보여요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다른 코드에서 저렇게 사용하는 것을 보고 배열 복사 시 concat()
을 사용하는 것으로 오해했습니다.
배열 복사를 목적으로 하는 구문에서는 slice(0)
를 사용하는 것으로 수정하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
참고로 배열 전체 복사는 그냥 slice()
만 해도 돼요
리뷰 완료합니다~ |
Please check if the PR fulfills these requirements
fix #xxx[,#xxx]
, where "xxx" is the issue number)Description
Thank you for your contribution to TOAST UI product. 🎉 😘 ✨